core: transfer views to same workspace as on destroyed output #2294
Conversation
|
Is this not able to be fixed by only modifying the preserve-output plugin? Seems like that might be a better place for this to live. |
Perhaps, but why require the user to run the preserve output plugin to get the correct behaviour? Core obviously needs to do the right thing here in case the user is not running the plugin. And in fact the code does try to do the right thing, by putting the views in roughly the same place as they were on the previous output, scaled by the ratio of output sizes. This commit just makes it put the views on the right desktop as well. 😄 |
There was a problem hiding this comment.
This isn't ideal because move_view_to_output is used in other places as well, for example the oswitch plugin, and this will break the plugin.
I think the idea in general makes sense when destroying an output. It might make sense to add more flags to the function (instead of a bool). Maybe even add two overloads (to avoid breaking changes), one bool, one with flags, have one call the other :)
lcolitti
force-pushed
the
move-same-workspace
branch
2 times, most recently
from
71d21cb to
536d767
Compare
Done |
lcolitti
force-pushed
the
move-same-workspace
branch
from
536d767 to
b99cb04
Compare
lcolitti
force-pushed
the
move-same-workspace
branch
2 times, most recently
from
ffce329 to
9925d03
Compare
lcolitti
force-pushed
the
move-same-workspace
branch
from
9925d03 to
68bd7f4
Compare
|
@ammen99 could you take a look? Or should I open another PR, since this one is marked stale? |
| { | ||
| move_view_to_output(view, to, true); | ||
| unsigned flags = VIEW_TO_OUTPUT_FLAG_RECONFIGURE | VIEW_TO_OUTPUT_FLAG_SAME_WORKSPACE; | ||
| move_view_to_output(view, to, flags); |
There was a problem hiding this comment.
I was thinking about whether this should be an option or not, but I suppose the new behavior is pretty much an improvement over the old behavior. Putting this comment here just in case someone really wants the old behavior, let us know and we can make it an option. But if nobody complains, there is no need for the option :)
lcolitti
force-pushed
the
move-same-workspace
branch
2 times, most recently
from
ce0330b to
1d7eeb1
Compare
src/core/output-layout.cpp
Outdated
| * enough to hold the windows of the largest output that is | ||
| * being removed. | ||
| */ | ||
| wf::dimensions_t max_size = {0, 0}; |
There was a problem hiding this comment.
Hey instead of this, have you tried simply using
[output:NOOP-1]
mode = 3840x2160in the config file? I would just prefer to let the user control the size of the biggest buffer we try to allocate ourselves. We have ran into issues with too big buffers before (example
Lines 152 to 157 in fcdc7b4
There was a problem hiding this comment.
In addition, the current implementation only sets the max size the first time the noop output is created - but the second time, maybe we have an even bigger window, we calculate the max_size, but noop_output might not be null and you will hit the same issue as before.
There was a problem hiding this comment.
Yes, setting it in the config file does work. Personally I don't think this is a great solution - it will be very hard for users to understand why their windows are sometimes positioned correctly if their screen is <= 1280x720, but not if it's bigger - but at least it's possible to configure things to make it work.
I've removed this commit from the patch series for now.
Why are you concerned that the output might be too large? It's (roughly) only as large as the largest output that was already configured, so that memory was already being used, and it (roughly) only sticks around for as long as there are no other outputs - which (likely) means that nothing else is using that memory. Or is it because it could be larger, for example if you remove one output that is 128x3840 and another output that is 2160x128, the noop output becomes 3840x2160, which uses much more memory?
src/api/wayfire/core.hpp
Outdated
| /** | ||
| * Change the view's output to new_output. | ||
| */ | ||
| void move_view_to_output(wayfire_toplevel_view v, wf::output_t *new_output, unsigned flags); |
There was a problem hiding this comment.
Last nitpick, I'd prefer uint32_t as the flags type
There was a problem hiding this comment.
Ah, yes - if it's an API I guess it's better for it to be a fixed size.
Currently, when an output is destroyed, views are transferred to the new output in roughly the same position as they were on the old output, but they are all transferred to the first workspace. This is annoying: if they were not on the first workspace, then the user must have moved them, and after they're transferred, the user will have to move them agein. Instead, move each view to the same workspace it was on before, or, if the new workspace set has a smaller grid, to the edge.
lcolitti
force-pushed
the
move-same-workspace
branch
from
1d7eeb1 to
b60a25b
Compare
3 participants
Currently, when an output is destroyed, views are transferred to the new output in roughly the same position as they were on the old output, but they are all transferred to the first workspace.
This is annoying: if they were not on the first workspace, then the user must have moved them, and after they're transferred, the user must will have to move them agein.
Instead, in the (common?) case that the old and the new output have the same number of workspaces, move each view to the same workspace it was on before.